feat(grpc-proxy): expose GET /info endpoint - #610
Conversation
📝 WalkthroughWalkthroughChangesgRPC proxy version metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant newProxyMux
participant golibversion
Client->>newProxyMux: GET /info
newProxyMux->>golibversion: Read service, version, and commit metadata
golibversion-->>newProxyMux: Build version metadata
newProxyMux-->>Client: JSON response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
96e3c1f to
1e8b8a4
Compare
Signed-off-by: priyaselvaganesan <pselvaganesa@nvidia.com>
1e8b8a4 to
816fdd0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/invocation-plane-services/grpc-proxy/proxy/h2.go (1)
45-46: 📐 Maintainability & Code Quality | 🔵 TrivialVerify observability coverage for
/info.
createHttp2Serverappliesmiddleware.ApplyMiddlewareto the complete mux. Confirm that this middleware emits structured request logs, an inbound OpenTelemetry span, and RED metrics for/info. Add route-specific instrumentation only if the shared middleware excludes it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/invocation-plane-services/grpc-proxy/proxy/h2.go` around lines 45 - 46, Verify that the middleware applied to the complete mux in createHttp2Server provides structured request logging, inbound OpenTelemetry spans, and RED metrics for the /info route. Only add route-specific instrumentation if the shared middleware does not cover /info; otherwise leave the existing shared middleware unchanged.Sources: Coding guidelines, Path instructions
src/invocation-plane-services/grpc-proxy/proxy/info_test.go (1)
55-65: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftExercise the build-time metadata contract.
The test currently accepts
"unknown"as a valid result, so it cannot detect a broken linker mapping. Add one stamped test path that proves the expected service name, release version, and full SHA reach the shared handler.
src/invocation-plane-services/grpc-proxy/proxy/info_test.go#L55-L65: replace non-empty checks with assertions against controlled expected values.src/invocation-plane-services/grpc-proxy/BUILD.bazel#L46-L48: connect the test or a binary-level test to theService,Version, andGitHashx_defs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/invocation-plane-services/grpc-proxy/proxy/info_test.go` around lines 55 - 65, Replace the non-empty checks in the info handler test with assertions for controlled expected service, version, and full SHA values. In src/invocation-plane-services/grpc-proxy/proxy/info_test.go:55-65, define or reuse those expected values and validate each response field exactly. In src/invocation-plane-services/grpc-proxy/BUILD.bazel:46-48, wire the test or a binary-level test to the Service, Version, and GitHash x_defs so the stamped metadata reaches the shared handler.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/invocation-plane-services/grpc-proxy/proxy/h2.go`:
- Around line 45-46: Verify that the middleware applied to the complete mux in
createHttp2Server provides structured request logging, inbound OpenTelemetry
spans, and RED metrics for the /info route. Only add route-specific
instrumentation if the shared middleware does not cover /info; otherwise leave
the existing shared middleware unchanged.
In `@src/invocation-plane-services/grpc-proxy/proxy/info_test.go`:
- Around line 55-65: Replace the non-empty checks in the info handler test with
assertions for controlled expected service, version, and full SHA values. In
src/invocation-plane-services/grpc-proxy/proxy/info_test.go:55-65, define or
reuse those expected values and validate each response field exactly. In
src/invocation-plane-services/grpc-proxy/BUILD.bazel:46-48, wire the test or a
binary-level test to the Service, Version, and GitHash x_defs so the stamped
metadata reaches the shared handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ada38786-1ec9-4e31-bfd5-1271ae5e28ab
⛔ Files ignored due to path filters (1)
src/invocation-plane-services/grpc-proxy/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
src/invocation-plane-services/grpc-proxy/BUILD.bazelsrc/invocation-plane-services/grpc-proxy/go.modsrc/invocation-plane-services/grpc-proxy/proxy/BUILD.bazelsrc/invocation-plane-services/grpc-proxy/proxy/h2.gosrc/invocation-plane-services/grpc-proxy/proxy/info_test.gotools/scripts/test/test-workspace-status.shtools/workspace_status.sh
|
Responses to the two nitpick comments from the review.
|
|
🎉 This PR is included in version nvcf-grpc-proxy-v1.32.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
TL;DR
Adds
GET /infoto grpc-proxy. It serves service, version, and commit via go-lib's sharedgolibversion.Handler().Additional Details
/info's fields come fromService,Version, andGitHashx_defs stamped at build time, added to the existingnvcf-grpc-proxy_libmap alongsideproxy.version.GitHashuses{STABLE_GIT_COMMIT_FULL}, a stamp key this PR adds to the sharedtools/workspace_status.sh(with a test) for the full commit SHA./inforegisters on the http/2 traffic mux next to/and/health(proxy/h2.go,newProxyMux), so it runs through the existingotelhttpmetrics, traces, and zap logging like the proxied traffic.newProxyMuxto make the routes unit-testable.For QA
From the repo root, run
bazel build //src/invocation-plane-services/grpc-proxy:image.tar --stamp. The binary embedsnvcf-grpc-proxy(Service),mr-<sha>(Version), and the full 40-char commit SHA. Unit tests cover GET returning 200 JSON and non-GET returning 405 withAllow: GETand an empty body.Deployed the stamped image to a local k3d cluster and curled the endpoint through the pod:
The commit field matched the deployed commit SHA.
For the Reviewer
golibversion.Handler()). Pattern follows helm-reval feat(helm-reval): exposeGET /infoendpoint #275.Issues
Relates to #315
Dependencies
Adds go-lib
v0.0.0-20260728185909-afca4ec2fb26. Go MVS pulls otel and relatedgolang.org/xdeps forward as go-lib requires.Summary by CodeRabbit
/infoendpoint that returns service metadata in JSON./,/health, and/info./infoendpoint and commit hash reporting.